-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUGFIX beta] Add warning for “deep @each” usage in dependent keys #12847
Conversation
LGTM |
Can you rebase? |
☔ The latest upstream changes (presumably #12848) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -257,6 +257,12 @@ ComputedPropertyPrototype.property = function() { | |||
`Please refactor from \`Ember.computed('${property}', function() {});\` to \`Ember.computed('${property.slice(0, -6)}.[]', function() {})\`.`, | |||
property.slice(-5) !== '@each' | |||
); | |||
warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it not be an assertion? also, should we say the workaround is to create an intermediary computed property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its an assertion it may break people's apps who are using it today without knowing that it doesn't work. 👍 for suggesting an intermediate CP in the meantime.
Other than I think we should consider it an error since it doesn't actually work to do this, this looks good to me. |
I don't think it can be an error until it's deprecated, etc. even though it doesn't work as expected. Would cause mega emo for little win. |
04427e4
to
4cd4615
Compare
Rebased and applied @krisselden and @mmun suggestions. |
[BUGFIX beta] Add warning for “deep @each” usage in dependent keys
Thanks @jgwhite! |
<3 |
Thanks much for this patch! It has cost us many hours and bugs, and we are still concerned that it would be popping up from time to time. Although, I think that an error would be more appropriate than a warning... since it doesn't even work. But this is great for starters... The user base would MUCH appreciate if these dependency variants could actually be supported. Is this being considered? |
@barneywilliams it would require a champion, I don't believe anyone currently is. Maybe someday |
@stefanpenner Fully understand and agree. Maybe I can carve out the motivation to investigate and maybe take a stab at it some night or weekend... |
FWIW, I think we all generally agree that we would like I'm not really sold on making |
Ya, its possible but ends up being quite costly to maintain. Maybe after/while the next push on chains/references happens. |
Thanks for the feedback guys. I have another co-worker/ember-dev that is willing to pitch in as well, so maybe we can actually make this happen. I agree/understand the additional complexities with nested I am going to file a request and link back to this post (for reference). Maybe we can drum up more support there as well. |
This will warn also on hidden usages of this like computed.filterBy("array","state.isActive",true)? |
@cibernox looks like the array macros use |
This patch also warns for dep keys of the form |
@barneywilliams all of those situations are accomplishable with two computed properties. It's possible if you need it enough to write a computed property of your own that generates the necessary first step computed for you. Not sure how well it would perform, but would cut down some boiler plate from time to time. |
Chris, that is a good thing to point out on this thread. That coupled with the new warning is definitely buttons it all up better and may be a decent place to settle. Aesthetically, it bugs me to write CPs that are only used internally, especially to work around limitations. Although, Ember is very powerful as it is and am glad there are solutions, albeit workarounds, for these limitations.
|
Actually interested on the last case @jgwhite explained. I have a hierarchy of classes and I use some CP's coming from a flat-map operation. I have a class const C = Ember.Object.extend({
prop: Ember.computed(/* something that resolves to an array */)
});
const B = Ember.Object.extend({
cs: collectionOfInstancesOfC,
prop: Ember.computed(
'[email protected].[]',
function() { flatMap('cs', 'prop'); }),
});
const A = Ember.Object.extend({
bs: collectionOfInstancesOfB,
prop: Ember.computed(
'[email protected].[]',
function() { flatMap('bs', 'prop'); }),
}); Which would be the right way to do this to avoid the warning? |
Adds a warning when defining dependent keys of the form
[email protected]
or[email protected][email protected]
as described in the guides.Fixes #12840.